-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix accessibility event properties for TextInput #24641
Fix accessibility event properties for TextInput #24641
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @elucaswork in 61c5e35. When will my fix make it into a release? | Upcoming Releases |
Summary: When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash. <p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p> [iOS] [Fixed] - Fix accessibility event properties for `TextInput` Pull Request resolved: #24641 Differential Revision: D15120211 Pulled By: cpojer fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
@@ -505,13 +505,6 @@ - (CGSize)sizeThatFits:(CGSize)size | |||
return fittingSize; | |||
} | |||
|
|||
#pragma mark - Accessibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this particular change is simply wrong/unreasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear: Removing this turns off all accessibility props of (by redirecting them to the wrapper view).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shergin my idea of deleting this reactAccessibilityElement
was to make RCTBaseTextInputView
use reactAccessibilityElement
inherited from RCTView
(the wrapper).
When I opened this PR I tested TextField
passing those events set from RCTViewManager, and it was working. But I might have missed something :-/.
Now I'll need to make more tests, especially because I this commit needed to be reverted (probably because of this detox failure https://circleci.com/gh/facebook/react-native/88586, right @grabbou?). I just don't know yet why this commit caused a regression into Switch
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think Detox couldn't find a TextInput on the top of the app (by its accessibility label), to type <switch />
and select that example.
So definitely related.
Summary: When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash. <p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p> [iOS] [Fixed] - Fix accessibility event properties for `TextInput` Pull Request resolved: #24641 Differential Revision: D15120211 Pulled By: cpojer fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
Summary: When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash. <p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p> [iOS] [Fixed] - Fix accessibility event properties for `TextInput` Pull Request resolved: facebook/react-native#24641 Differential Revision: D15120211 Pulled By: cpojer fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
Summary: This sync includes the following changes: - **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>// - **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>// - **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>// - **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>// - **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>// - **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>// - **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>// - **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>// - **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>// - **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>// - **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>// - **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>// - **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>// - **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>// - **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>// - **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>// - **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>// - **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>// - **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>// - **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>// Changelog: [General][Changed] - React Native sync for revisions bd4784c...d300ceb jest_e2e[run_all_tests] Reviewed By: cortinico, kacieb Differential Revision: D36874368 fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
Summary
When a
TextInput
receives any accessibility event prop (onAccessibilityTap
,onMagicTap
,onAccessibilityEscape
,onAccessibilityEscape
), causes a crash.Changelog
[iOS] [Fixed] - Fix accessibility event properties for
TextInput
Test Plan
The following component shouldn't crash the app.